PoC: Resolution CLI #246

m1kola · 2023-06-01T16:52:42Z

Description

This is a PoC for #206.

Right now the tool is capable of resolving a package without a cluster: it is possible to provide a FBC (dir or an image) into the tool as well as a directory containing manifests with Operator objects.

The resolution tool will use Operator from the input manifests and consider them required (as if they were installed on a cluster).

Once we merge this, the next steps will be:

Gather feedback from the community using this GitHub discussion - Off Cluster CLI Tools #262
Plan next iterations based on feedback

Also there are a few things going in parallel. For example, we will likely change significantly how we build resolution problems (see operator-framework/deppy#96 and relevant RFC). Once we have more or less stable way to build the resolution problem - we will be able to create some sort of library to share between operator-controller and resolution CLI.

Example usage:

# Resolve prometheus from catalog quay.io/operatorhubio/catalog:latest from default channel
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus
quay.io/operatorhubio/prometheus@sha256:85decebfdfe993484834bd722d93fb44725b3a56cb4f9ebc4e2fd398bdcbad54

# Resolve prometheus from catalog dir ~/Downloads/index from default channel
$ go run ./cmd/resolutioncli/ -index-ref ~/Downloads/index -package-name prometheus
quay.io/operatorhubio/prometheus@sha256:85decebfdfe993484834bd722d93fb44725b3a56cb4f9ebc4e2fd398bdcbad54

# Resolve prometheus version 0.65.1 from default channel
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -package-version 0.65.1
quay.io/operatorhubio/prometheus@sha256:85decebfdfe993484834bd722d93fb44725b3a56cb4f9ebc4e2fd398bdcbad54

# Resolve prometheus version 0.47.0 from default channel
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -package-version 0.47.0
quay.io/operatorhubio/prometheus@sha256:47c659b352bbc4b75b64fbe0077d19afaea80cf691c62d803d44fdfc88dc9c6f

# Resolve prometheus version 0.47.0 from stable channel which doesn't exist (beta is default channel)
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -package-version 0.47.0 -package-channel stable
package 'prometheus' at version '0.47.0' in channel 'stable' not found
exit status 1

# Resolve prometheus version 0.47.0 from beta channel
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -package-version 0.65.1 -package-channel beta
quay.io/operatorhubio/prometheus@sha256:85decebfdfe993484834bd722d93fb44725b3a56cb4f9ebc4e2fd398bdcbad54

# Resolve prometheus from default channel and reading input manifests from ./operator-input/ dir
# Note: at the moment, resolution tool only takes Operator kind as an input and considers it to be
# a required package.
$ mkdir operator-input
$ cat <<EOT > operator-input/operator_v1alpha1.yaml
apiVersion: operators.operatorframework.io/v1alpha1
kind: Operator
metadata:
  name: operator-sample
spec:
  packageName: argocd-operator
EOT
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -input-dir ./operator-input/
quay.io/operatorhubio/prometheus@sha256:85decebfdfe993484834bd722d93fb44725b3a56cb4f9ebc4e2fd398bdcbad54

# Resolve prometheus from default channel and reading input manifests from ./operator-input/ dir
# and prometheus package already exists in the input (e.g. already installed on the cluster)
$ mkdir operator-input
$ cat <<EOT > operator-input/operator_v1alpha1.yaml
apiVersion: operators.operatorframework.io/v1alpha1
kind: Operator
metadata:
  name: operator-sample
spec:
  packageName: prometheus
  version: 0.47.0
EOT
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -input-dir ./operator-input/
duplicate identifier "required package prometheus" in input
exit status 1

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

cmd/resolutioncli/fbcsource.go

cmd/resolutioncli/entity_source.go

everettraven

Overall this looks good to me with the idea in mind that this still has a good bit of follow up work to do to get it production ready. I had a minor nit but nothing that should block this PR IMO.

I'm not very well versed in the variable source / deppy stuff so I'd definitely prefer someone more familiar with that review it prior to merging in case I've missed something.

Great work @m1kola !

everettraven · 2023-06-28T17:32:28Z

cmd/resolutioncli/main.go

+func validateFlags(packageName, indexRef string) error {
+	if packageName == "" {
+		return fmt.Errorf("missing required -%s flag", flagNamePackageName)
+	}
+
+	if indexRef == "" {
+		return fmt.Errorf("missing required -%s flag", flagNameIndexRef)
+	}
+
+	return nil
+}


Nit: If these flags are required would it make more sense to make them positional arguments? IMO flags should be reserved for configurable options.

I noticed we aren't using spf13/cobra for building the CLI commands, but it has some nice support for setting required number of arguments that we could use here and automatically generates a pretty nice help message that could be used to show the required positional arguments.

I'm a fan of explicit arguments; it can get confusing with positional unless there's an explicit logic (e.g. kubectl's verb type [name])

Package name probably makes sense as a positional argument and everything else as flags. But I suggest that we leave it as is for now and come back to discuss details of the interface once start work on finalising this CLI.

We are not using cobra right now, but I think we should use it in the production-ready version.

tmshort

Some small changes (mostly copyright years). However, I would like to see this incorporated in the Makefile as either a target that's automatically built, or a separate target.

internal/resolution/variable_sources/olm/composite.go

tmshort · 2023-06-27T20:50:43Z

internal/resolution/variable_sources/olm/olm.go

 	}

-	// build variable source pipeline
-	variableSource := crdconstraints.NewCRDUniquenessConstraintsVariableSource(bundlesanddependencies.NewBundlesAndDepsVariableSource(inputVariableSources...))


This appears to add CRD Uniqueness and Bundle variable sources to the list, but I don't see them in the new code?
Unless it's wrapped by NewOLMVariableSource, but I don't see it being used.
I guess it's the NewOLMVariableSource() "constructor" that's adding these, rather than the GetVariables?

I had to remove this chaining within the variable source becase for off-cluster usage we need a bit different set of variable soruces (e.g. we need a variable source which creates a variable from CLI inputs).

instead of chaining I now use NestedVariableSource in NewOLMVariableSource (note after #273 it is NewOperatorVariableSource). NewOLMVariableSource is used in on-cluster operator and it is not in this diff (I did not change the calling code).

https://github.com/operator-framework/operator-controller/blob/9a1a15d7d7c7e069dab21ff5930052d13553e1fc/cmd/manager/main.go#L108C19-L108C19

But NewCRDUniquenessConstraintsVariableSource and NewBundlesAndDepsVariableSource are also used in off-cluster resolution here within olm.NestedVariableSource.

I suspect NestedVariableSource and probably whole composite.go will go away once we switch to the solution proposed in the RFC.

cmd/resolutioncli/variable_source.go

cmd/resolutioncli/entity_source.go

cmd/resolutioncli/input_manifests.go

cmd/resolutioncli/main.go

tmshort · 2023-06-28T14:21:59Z

cmd/resolutioncli/main.go

+func init() {
+	utilruntime.Must(clientgoscheme.AddToScheme(scheme))
+	utilruntime.Must(operatorsv1alpha1.AddToScheme(scheme))
+	utilruntime.Must(rukpakv1alpha1.AddToScheme(scheme))
+	utilruntime.Must(catalogd.AddToScheme(scheme))
+}


Is the init() function necessary, give that this is a CLI?

We can do it in main I think, but I was following the pattern from operator-controller's main.go:

operator-controller/cmd/manager/main.go

Lines 49 to 57 in ebc5cfe

func init() {

utilruntime.Must(clientgoscheme.AddToScheme(scheme))

utilruntime.Must(operatorsv1alpha1.AddToScheme(scheme))

utilruntime.Must(rukpakv1alpha1.AddToScheme(scheme))

utilruntime.Must(catalogd.AddToScheme(scheme))

//+kubebuilder:scaffold:scheme

}

tmshort · 2023-06-28T17:40:52Z

cmd/resolutioncli/main.go

+func validateFlags(packageName, indexRef string) error {
+	if packageName == "" {
+		return fmt.Errorf("missing required -%s flag", flagNamePackageName)
+	}
+
+	if indexRef == "" {
+		return fmt.Errorf("missing required -%s flag", flagNameIndexRef)
+	}
+
+	return nil
+}


I'm a fan of explicit arguments; it can get confusing with positional unless there's an explicit logic (e.g. kubectl's verb type [name])

codecov · 2023-06-29T13:57:43Z

Codecov Report

Merging #246 (5aa27e5) into main (b7a737a) will increase coverage by 0.86%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
+ Coverage   76.13%   77.00%   +0.86%     
==========================================
  Files          13       15       +2     
  Lines         662      687      +25     
==========================================
+ Hits          504      529      +25     
  Misses        129      129              
  Partials       29       29

Impacted Files	Coverage Δ
internal/controllers/variable_source.go	`100.00% <100.00%> (ø)`
internal/resolution/variablesources/composite.go	`100.00% <100.00%> (ø)`
internal/resolution/variablesources/operator.go	`78.94% <100.00%> (+2.47%)`	⬆️

Signed-off-by: Mikalai Radchuk <[email protected]>

m1kola

@tmshort I think I addressed your feedback + made sure this PR doesn't decrease test coverage.

However, I would like to see this incorporated in the Makefile as either a target that's automatically built, or a separate target.

I did not want to add this into makefile or goreleaser because it is still a PoC and a lot will change. Didn't want to make it "official" for now.

But if you think that we should still do that - I suggest that we do it in a separate PR.

m1kola · 2023-06-30T09:48:57Z

internal/controllers/variable_source.go

+	"github.com/operator-framework/operator-controller/internal/resolution/variablesources"
+)
+
+func NewVariableSource(cl client.Client) variablesources.NestedVariableSource {


I moved it here from internal/resolution/variablesources/. The idea is that building blocks are in internal/resolution/variablesources/ and this composite variable source is close to where it will be used (in controller or in the off-cluster.

m1kola · 2023-06-30T10:47:49Z

internal/resolution/variablesources/operator_test.go

@@ -109,86 +87,11 @@ var _ = Describe("OLMVariableSource", func() {
 		})))
 	})

-	It("should produce BundleVariables variables", func() {


These removed tests are mostly to cover inner variables sources which are no longer hardcoded inside (see this) and have their own tests (bundles_and_dependencies_test.go, required_package_test.go, etc).

tmshort · 2023-06-30T18:34:05Z

I would like to see a mMakefile entry for this, but perhaps not now.

Bumps [docker/login-action](https://github.com/docker/login-action) from 2 to 3. - [Release notes](https://github.com/docker/login-action/releases) - [Commits](docker/login-action@v2...v3) --- updated-dependencies: - dependency-name: docker/login-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 1, 2023

m1kola force-pushed the resolution-cli branch 3 times, most recently from 7382e7d to bdb76b4 Compare June 2, 2023 16:05

m1kola commented Jun 6, 2023

View reviewed changes

cmd/resolutioncli/fbcsource.go Outdated Show resolved Hide resolved

m1kola force-pushed the resolution-cli branch 5 times, most recently from 93e0010 to 2ec7e6c Compare June 8, 2023 14:47

m1kola mentioned this pull request Jun 8, 2023

Makes OLMVariableSource responsible for fetching #245

Merged

4 tasks

m1kola force-pushed the resolution-cli branch from 2ec7e6c to b785cc2 Compare June 8, 2023 15:09

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2023

m1kola force-pushed the resolution-cli branch from b785cc2 to d8f30a9 Compare June 9, 2023 09:51

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2023

m1kola force-pushed the resolution-cli branch from d8f30a9 to b4029e8 Compare June 9, 2023 10:23

m1kola commented Jun 9, 2023

View reviewed changes

cmd/resolutioncli/entity_source.go Show resolved Hide resolved

m1kola force-pushed the resolution-cli branch 3 times, most recently from 0e10978 to 1c3a3ad Compare June 13, 2023 08:12

m1kola mentioned this pull request Jun 20, 2023

Dependency Library + CLI #206

Closed

m1kola force-pushed the resolution-cli branch 2 times, most recently from c425615 to 1c0b2db Compare June 21, 2023 11:36

m1kola marked this pull request as ready for review June 21, 2023 13:13

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 21, 2023

kevinrizza assigned kevinrizza and unassigned kevinrizza Jun 21, 2023

m1kola force-pushed the resolution-cli branch from 1c0b2db to 63474c8 Compare June 23, 2023 12:27

everettraven approved these changes Jun 28, 2023

View reviewed changes

tmshort requested changes Jun 28, 2023

View reviewed changes

m1kola force-pushed the resolution-cli branch 2 times, most recently from 8fb5a8a to 7e3c73a Compare June 29, 2023 13:56

m1kola force-pushed the resolution-cli branch 6 times, most recently from a28b436 to 90db305 Compare June 30, 2023 10:44

m1kola added 2 commits June 30, 2023 11:58

Resolution CLI POC: offline catalogs

f8a5422

Signed-off-by: Mikalai Radchuk <[email protected]>

Resolution CLI POC: offline input

5aa27e5

Signed-off-by: Mikalai Radchuk <[email protected]>

m1kola force-pushed the resolution-cli branch from 90db305 to 5aa27e5 Compare June 30, 2023 10:58

m1kola commented Jun 30, 2023

View reviewed changes

m1kola requested a review from tmshort June 30, 2023 11:21

tmshort approved these changes Jun 30, 2023

View reviewed changes

m1kola merged commit 70407ba into operator-framework:main Jul 3, 2023

m1kola deleted the resolution-cli branch July 3, 2023 08:28

ankitathomas mentioned this pull request Jun 6, 2024

CatalogSource CRD needed in installation steps #145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PoC: Resolution CLI #246

PoC: Resolution CLI #246

m1kola commented Jun 1, 2023 •

edited

Loading

everettraven left a comment

everettraven Jun 28, 2023

tmshort Jun 28, 2023

m1kola Jun 29, 2023

tmshort left a comment

tmshort Jun 27, 2023

m1kola Jun 29, 2023

tmshort Jun 28, 2023

m1kola Jun 29, 2023

tmshort Jun 28, 2023

codecov bot commented Jun 29, 2023 •

edited

Loading

m1kola left a comment

m1kola Jun 30, 2023

m1kola Jun 30, 2023

tmshort commented Jun 30, 2023

	func init() {
	utilruntime.Must(clientgoscheme.AddToScheme(scheme))

	utilruntime.Must(operatorsv1alpha1.AddToScheme(scheme))
	utilruntime.Must(rukpakv1alpha1.AddToScheme(scheme))
	utilruntime.Must(catalogd.AddToScheme(scheme))

	//+kubebuilder:scaffold:scheme
	}

PoC: Resolution CLI #246

PoC: Resolution CLI #246

Conversation

m1kola commented Jun 1, 2023 • edited Loading

Description

Example usage:

Reviewer Checklist

everettraven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 29, 2023 • edited Loading

Codecov Report

m1kola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort commented Jun 30, 2023

m1kola commented Jun 1, 2023 •

edited

Loading

codecov bot commented Jun 29, 2023 •

edited

Loading